fix: preserve reasoning_content for OpenAI-compatible reasoning models#604
Conversation
Models like Moonshot kimi-k2.5 and DeepSeek-R1 return a reasoning_content field in assistant messages. When thinking is enabled, the API requires this field to be echoed back in subsequent requests. PicoClaw was silently dropping it, causing 400 errors on tool-call round-trips. - Add ReasoningContent to Message and LLMResponse types - Parse reasoning_content in openai_compat parseResponse() - Carry reasoning_content through assistant tool-call messages - Add unit test for reasoning_content parsing Fixes sipeed#588
alexhoshina
left a comment
alexhoshina
left a comment
There was a problem hiding this comment.
LGTM! Ready for @yinwm to merge.
Huaaudio
left a comment
Huaaudio
left a comment
There was a problem hiding this comment.
Second. Preserving reasoning content is quite necessary for agent iterations.
nikolasdehor
left a comment
nikolasdehor
left a comment
There was a problem hiding this comment.
Clean fix for a real problem. Reasoning models (DeepSeek-R1, kimi-k2.5, QwQ) return reasoning_content in assistant messages and require it echoed back on subsequent turns. Without this, tool-call round-trips fail with 400 errors because the API expects the field to be present.
What is correct:
- ReasoningContent field added to both Message and LLMResponse in protocoltypes/types.go with omitempty so it is invisible for non-reasoning models.
- Parsed from the API response in openai_compat/provider.go parseResponse().
- Carried through the assistant message construction in loop.go when tool calls are present -- this is the critical path, since the reasoning content needs to survive the assistant->tool->assistant round-trip.
- The test covers the important case: reasoning content + tool calls in the same response.
Edge cases to consider (non-blocking):
- Streaming responses: If the codebase has a streaming path (SSE), reasoning_content may arrive as a separate delta field. This PR handles the non-streaming path correctly. If streaming is added later, it will need the same treatment.
- Anthropic's thinking field: Anthropic uses a different field name (thinking blocks in the content array). This is correctly scoped to openai_compat only, so no conflict.
- Cost/token implications: reasoning_content can be very large (thousands of tokens of chain-of-thought). Echoing it back on every turn means context grows faster. This is working-as-designed per the API spec, but worth noting for users of reasoning models with limited context windows.
The test is well-structured -- it validates that reasoning content, regular content, and tool calls are all parsed correctly from a single response. Approving.
|
Hi @yinwm, just a gentle reminder about the PR when you have a moment 🙂 |
|
LGTM |
|
@winterfx Good find on reasoning_content being silently dropped. Getting 400 errors on tool-call round-trips with models like kimi-k2.5 must have been frustrating to debug. Carrying the field through the full message chain is the right fix. We're putting together the PicoClaw Dev Group on Discord for contributors to chat and collaborate. If you'd like to join, send an email to |
…sing fix: preserve reasoning_content for OpenAI-compatible reasoning models
7 participants
Models like Moonshot kimi-k2.5 and DeepSeek-R1 return a reasoning_content field in assistant messages. When thinking is enabled, the API requires this field to be echoed back in subsequent requests. PicoClaw was silently dropping it, causing 400 errors on tool-call round-trips.
Fixes #588
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist